Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow separate strong emphasis marker #103

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Samasaur1
Copy link
Contributor

@Samasaur1 Samasaur1 commented Feb 3, 2023

Bug/issue #, if applicable: N/A

Summary

As it stands, the selection of an emphasisMarker in MarkupFormatter.Options determines the marker used for both emphasis and strong emphasis. However, the Markdown spec allows one to, and I prefer to, use different markers for emphasis as compared to strong emphasis. For example: _emphasis_ and **strong emphasis**.

This PR adds an Optional parameter to MarkupFormatter.Options that allows one to specify the marker used for strong emphasis. If omitted, it takes on the value of the marker used for normal emphasis.

Dependencies

None

Testing

I've updated the bundled markdown-tool as well, so you can simply run:

echo "Here's some **strong emphasis** and some _normal emphasis_" | swift run markdown-tool format --emphasis-marker _ --strong-emphasis-marker \*

which will replicate the input as output. But if one runs:

echo "Here's some **strong emphasis** and some _normal emphasis_" | swift run markdown-tool format --emphasis-marker _

the output is Here's some __strong emphasis__ and some _normal emphasis_ — i.e., the previous behavior is honored.

Steps:

  1. Clone this PR/my repository
  2. Run the above commands

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

The ./bin/test script already fails for me on main, but I haven't introduced any new issues.

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Jul 13, 2023

@swift-ci please test

@Samasaur1 Samasaur1 force-pushed the allow-separate-strong-emphasis-marker branch from b9f1384 to 7be8670 Compare October 10, 2024 01:55
@Samasaur1
Copy link
Contributor Author

@Kyle-Ye @QuietMisdreavus I've just rebased this PR on master, and all the tests (at least the ones that I can see) now pass. Would it be possible to get this merged?

@Kyle-Ye Kyle-Ye self-requested a review October 10, 2024 03:01
@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 10, 2024

@swift-ci please test

Copy link
Contributor

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown spec allows one to, and I prefer to, use different markers for emphasis as compared to strong emphasis.

Could you add the spec link which describe this feature? Thanks.

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 10, 2024

We are actually using GFM. So I believe we should refer into this spec

@Samasaur1
Copy link
Contributor Author

I was just about to link that, yeah

Copy link
Contributor

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. Leave it to @QuietMisdreavus for the final merge.

One more issue is for "___a___" or "***a***" input. If we specify emphasisMarker to be "_" and strongEmphasisMarker to be "*".

The result would be "_**a**_".

The order is expected but I do not see spec covers such usage. Can you provide some insights on this?

@Samasaur1
Copy link
Contributor Author

One more issue is for "___a___" or "***a***" input. If we specify emphasisMarker to be "_" and strongEmphasisMarker to be "*".

The result would be "_**a**_".

The order is expected but I do not see spec covers such usage. Can you provide some insights on this?

What specifically is your question? The ordering or whether it is allowed to have two different types of delimiters next to each other?

The ordering should be addressed by Example 476, and the fact that you are allowed to have different delimiters right next to each other is addressed by Example 472

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 10, 2024

What specifically is your question? The ordering or whether it is allowed to have two different types of delimiters next to each other?

different emphasis marker and strong emphasis right next to each other

The ordering should be addressed by Example 476

I have no problem of the ordering as it has been explicitly stated in the spec "An interpretation <em><strong>...</strong></em> is always preferred to <strong><em>...</em></strong>".

and the fact that you are allowed to have different delimiters right next to each other is addressed by Example 472

No. Example 472 does not address my issue. I have not seen any spec statement or example of "<em><strong>...</strong></em>" written as "_**{content}**_" or "*__{content}__*".

@Samasaur1
Copy link
Contributor Author

Ah, I see what you are asking. I'll give two examples.

  1. _**a**_ (which GitHub renders properly as a)

In this case, the first _ is a left-flanking delimiter run because it is a delimiter run 1) not followed by unicode whitespace and 2b) followed by a punctuation character and preceded by unicode whitespace (the start of line). The first ** is a left-flanking delimiter run because it is a delimiter run 1) not followed by unicode whitespace and 2a) not followed by a punctuation character.

The second ** is a right-flanking delimiter run because it is a delimiter run 1) not preceded by unicode whitespace and 2a) not preceded by a punctuation character. The second _ is a right-flanking delimiter run because it is a delimiter run 1) not preceded by unicode whitespace and 2b) preceded by a punctuation character and followed by unicode whitespace (the end of line).

The first _ can open emphasis because it is part of a left-flanking delimiter run and a) not part of a right-flanking delimiter run. The first ** can open strong emphasis because it is part of a left flanking delimiter run.

The second ** can close strong emphasis because it is part of a right flanking delimiter run. The second _ can close emphasis because it is part of a right-flanking delimiter run and a) not part of a left-flanking delimiter run

  1. "_**a**_" (which GitHub renders properly as "a")

In this case, the first _ is a left-flanking delimiter run because it is a delimiter run 1) not followed by unicode whitespace and 2b) followed by a punctuation character and preceded by a punctuation character. The first ** is a left-flanking delimiter run because it is a delimiter run 1) not followed by unicode whitespace and 2a) not followed by a punctuation character.

The second ** is a right-flanking delimiter run because it is a delimiter run 1) not preceded by unicode whitespace and 2a) not preceded by a punctuation character. The second _ is a right-flanking delimiter run because it is a delimiter run 1) not preceded by unicode whitespace and 2b) preceded by a punctuation character and followed by a punctuation character.

The first _ can open emphasis because it is part of a left-flanking delimiter run and a) not part of a right-flanking delimiter run. The first ** can open strong emphasis because it is part of a left flanking delimiter run.

The second ** can close strong emphasis because it is part of a right flanking delimiter run. The second _ can close emphasis because it is part of a right-flanking delimiter run and a) not part of a left-flanking delimiter run

@Samasaur1
Copy link
Contributor Author

@Kyle-Ye hopefully that addresses your question. I do wish that they had examples, though

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 10, 2024

@Kyle-Ye hopefully that addresses your question. I do wish that they had examples, though

Got it. They are indeed grammatically correct. Just hoping we go a example in the spec though. LOL

@Samasaur1
Copy link
Contributor Author

I think that latest commit should address your other concern — let me know if you'd like any other changes to be made

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 10, 2024

@swift-ci test

@Kyle-Ye Kyle-Ye self-requested a review October 10, 2024 07:04
Copy link
Contributor

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@QuietMisdreavus
Copy link
Contributor

I feel like combined emphasis and strong emphasis should all be rendered with the strong emphasis marker, but that requires that visitEmphasis checks its immediate parent and/or child to see if it's part of a combination emphasis.

That would look something like this:

    public mutating func visitEmphasis(_ emphasis: Emphasis) {
        let emphasisMarker: String
        if let parent = emphasis.parent as? Strong, parent.childCount == 1 {
            emphasisMarker = formattingOptions.strongEmphasisMarker.rawValue
        } else if emphasis.childCount == 1, emphasis.child(at: 0) is Strong {
            emphasisMarker = formattingOptions.strongEmphasisMarker.rawValue
        } else {
            emphasisMarker = formattingOptions.emphasisMarker.rawValue
        }
        print(emphasisMarker, for: emphasis)
        descendInto(emphasis)
        print(emphasisMarker, for: emphasis)
    }

And then adding more tests to make sure that combination emphasis works as expected.

This way, we don't have to worry about semantics surrounding mixed emphasis markers. Even though we correctly parse the mixed result, it will look better in the rendered output to have these mixed emphasis spans use the same marker, even if we've been given separate emphasis markers.

@Samasaur1
Copy link
Contributor Author

I feel like combined emphasis and strong emphasis should all be rendered with the strong emphasis marker

I think this would be unexpected behavior. When the strong emphasis marker and normal emphasis marker are both set, the former should be used for strong emphasis, and the latter should be used for normal emphasis.

This way, we don't have to worry about semantics surrounding mixed emphasis markers. Even though we correctly parse the mixed result, it will look better in the rendered output to have these mixed emphasis spans use the same marker, even if we've been given separate emphasis markers.

Personally, I would still prefer my output to use different strong and normal emphasis markers even when they are both in use. I think this PR shouldn't include special cases where it diverges from the behavior one would expect when specifying two different emphasis markers

@Samasaur1
Copy link
Contributor Author

Unless there are any other concerns, would it be possible to merge this?

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 19, 2024

+1. cc @QuietMisdreavus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants